Offload Oxide scanning to separate process #1471
Conversation
|
I can pull it down at work on my windows machine tomorrow to try it out and report back! |
|
So it looks like this isn't possible from my (somewhat limited) testing. Node supposedly handles the unloading of these files but I'm guessing it just doesn't on Windows. If you run this script: let mod = require("./node_modules/@tailwindcss/oxide-win32-arm64-msvc/tailwindcss-oxide.win32-arm64-msvc.node")
console.log(mod);
mod = null
delete require.cache["./node_modules/@tailwindcss/oxide-win32-arm64-msvc/tailwindcss-oxide.win32-arm64-msvc.node"];
setTimeout(() => {}, 60_000)and then try to run |
thecrypticace
force-pushed
the
fix/issue-1470
branch
from
b8416c7 to
21c7dca
Compare
thecrypticace
changed the title
thecrypticace
force-pushed
the
fix/issue-1470
branch
from
00a0a62 to
cb0a073
Compare
|
@DustinsCode If you wanna give this PR a go it should work (does in my testing) |
thecrypticace
force-pushed
the
fix/issue-1470
branch
from
c655125 to
e730887
Compare
| // TODO: Can we find a way to not require a build first? | ||
| // let module = path.resolve(path.dirname(__filename), './oxide-helper.ts') |
There was a problem hiding this comment.
Yeah, I'm leaving this in as a note for me to look into later. Right now editing the helper file and re-running tests requires a rebuild. But this is "fine" for now.
CI already does this (b/c a few tests have to actually test against the built version for reasons).
These are expected if you are just using the plain `tailwindcss` package or don’t have `tailwindcss` installed locally at all — for example when using the Standalone CLI. Other errors should be surfaced though as they could indicate a problem.
thecrypticace
force-pushed
the
fix/issue-1470
branch
from
e730887 to
c537a5b
Compare
3 participants
Loading Oxide's .node file into the language server process / VSCode extension host on Windows marks that .node file as in use. When this happens you cannot completely delete node_modules (for example by running
npm ci).To work around this we'll fork a new process that can load Oxide, run its scan(s), and then exit. During initial project discovery we use a temporarily long lived process that persists for the duration of project discovery — which may include multiple Oxide scans across projects (and even versions). Once the project discovery has completed the process will exit. For any subsequent content scanning (e.g. when CSS changes) we'll spawn a new, temporary process for that individual scan.
This will ensure that, once the process has exited, the
.nodefile is no longer considered to be in-use and commands likenpm ciwill run properly.Commentary
Why not use
require.cache?Unfortunately, deleting entries from
require.cachedoes not unload the Oxide binary from the process address space.Why not worker threads
So, this might work but also might not. You'd still be loading it into the processes address space so there's a chance that as long as the process is open — whether the thread has exited or not — the
.nodefile would still be marked as in use.Additionally, we have some flags set when building Oxide that basically prevent it from unloading in worker threads due to some bugs in the Rust standard library. This applies to Linux only iirc so it shouldn't actually be a problem there but I'd rather keep the mechanism working consistently across operating systems.
Communication between processes
The main process and helper communicate using a JSON-RPC protocol — similar to the one used by language servers/clients — but without any initialization setup. This is an internal tool and the message format is not considered stable and may change in any future version.
Communication happens over an IPC channel provided by
child_process.fork(…). As far as I am aware, this uses private file descriptors shared between processes. No other process should be capable of "tricking" the helper into loading other.nodefiles into its address space. Only the ones we discover during NPM package resolution should ever be loaded. Even though the temporary helper isn't active for very long this was still a concern I had while developing this.Test Plan
There are automated tests that verify existing functionality still works but testing this specific scenario on Windows in an automated fashion with the current test setup would be a bit annoying so I did some additional manual testing:
npm ciin the terminal and watched it fail b/c of the Oxide.nodefilenpm cimultiple times to make sure it worked